Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mitigate potential Remote-Code-Execution caused by CVE-2021-44228 #1343

Merged
merged 1 commit into from
Dec 10, 2021

Conversation

Flole998
Copy link
Member

This is a mitigation for the issue mentioned in https://community.openhab.org/t/log4j-vulnerability/129863 until Karaf has been updated. However, we could leave this in "forever" since I don't see any reason why openHAB should ever use JNDI in logging-messages, so this adds an additional protection in case there are more shady things going on in the jndi code in log4j.

@Flole998 Flole998 requested a review from a team as a code owner December 10, 2021 18:26
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/log4j-vulnerability/129863/27

@kaikreuzer
Copy link
Member

@openhab/architecture-council I'd like to move the discussion from the forum to here.
I would clearly tend to merge this PR and possibly even backport it to 3.1. It is mentioned as an official mitigation of the issue and I couldn't see any logging issues on my (production) system, where I have done this change already.

@rkoshak, I understand for the forum discussions that you have concerns about it, so I don't want to overrule you, but hope that we can come to a joint view on the topic in the AC. If I got you right, you see the risk that some fundamental logging features might break, right? Could this be checked by testing it in various setups? If so, we could decide to merge it in the snapshot, roll it out and if issues turn up to revert it again, wdyt?

@rkoshak
Copy link
Contributor

rkoshak commented Dec 10, 2021

That is my primary concern. However, I think the risk of breaking the logging in the very short term is lower than the risk posed by the vulnerability. If nothing else, as shown in that thread, we should apply the fix to our Internet exposed demo server ASAP. We probably don't need logging there anyway.

I think at least @Flole998 has tested the mitigation and not found the logging to have broken so we have at least one test under our belt. It's probably low risk that anything that will impact day-to-day users is pretty low. I plan on testing it myself when I can get to where I can actually see my logs.

There might be some issues in DEBUG or TRACE logging hanging around that might crop up but I see no reason why we can't handle those as they arise. My biggest concern was that something in PAX or Karaf might break all logging if we disable all log4j2 Lookups.

In the long term, once we have the patched library available, I would recommend reversing the parameter that disables all lookups and apply some of the new parameters that Apache has implemented to limit what protocols and what machines it can reach out to. There is no reason OH needs to be using these features by default. But long term I do have some concerns disabling all lookups forever as we don't really have full control over the logging config from end to end and upstream may start using them without our knowledge and input.

@Flole998
Copy link
Member Author

I'd like to point out that I was able to do the first step of the exploit that connects to an LDAP Server on the openHAB Demo Installation at http://demo.openhab.org. While there is more involved into getting remote code execution in the currently published way (and modern java versions should block that) and while I am not disclosing yet how I've done it (and I haven't tried to do the other following steps as my intention was just to see if it could potentially be done and not to actually run code on that host), all internet-facing OH Installations that don't enforce authentication are potentially at risk currently. If someone comes up with a better way I wouldn't be mad at all if this doesn't get merged, it was the fastest fix I could come up with. In fact I'd prefer an updated karaf version, but there hasn't even be a timeframe announced yet as far as I can see, so maybe merge this, ignore the possible broken logging and delay the new release until karaf can be updated and make a new milestone and backport this. Or if someone wants to search through openHABs codebase it would even be possible to check if (and which) logging messages would break (something like .*\.log\(.*{..*}.*\) should be the correct RegEx, so basically anything that logs something with {somethingHere} ). This wouldn't include add-ons which are not included in any of the repos though, so you might miss some entries from unofficial add-ons.

Or maybe @wborn can do some of his magic and come up with a way to install the fixed version using bundle: install (and I believe there are config files to do that automagically), someone mentioned that it should be possible in the forum thread.

@kaikreuzer
Copy link
Member

we should apply the fix to our Internet exposed demo server ASAP

I just did that!

@kaikreuzer
Copy link
Member

Ok, thanks @rkoshak, than I guess we are fine to merge. I'll wait for another member of the @openhab/architecture-council to comment, but if there's no veto, let's go for this as a short-term fix and remove it again once we have a proper fix at hand.

Wdyt about doing a 3.1.1 patch release? Is it worth it or shall we just ask users to update to 3.2, once it is released on Dec 20?

@rkoshak
Copy link
Contributor

rkoshak commented Dec 10, 2021

Given how slowly people upgrade I think w need a 3.1.1 patch. I wouldn't even rule out a 3.0.1 patch for something like this. It only take a quick browsing of Shodan to realize how many people foolishly have a naked openHAB exposed to the Internet.

@Flole998
Copy link
Member Author

we should apply the fix to our Internet exposed demo server ASAP

I just did that!

I can confirm that it no longer tries to connect to me when I do what I have done before, so it is fixed/mitigated there aswell. Maybe it's worth to run a quick "grep {" on the logs there, just to see if it broke anything there aswell.

I think at least @Flole998 has tested the mitigation and not found the logging to have broken so we have at least one test under our belt.

Yes that is correct, I have verified that on my setup no "{" or "}" are in the log files and that I can no longer use the karaf-based test I described on the forum. So I have basically verified that I actually fixed what I wanted to fix and that I didn't break anything that I am using.

But long term I do have some concerns disabling all lookups forever as we don't really have full control over the logging config from end to end and upstream may start using them without our knowledge and input.

I agree with that, this was not designed as a "fix" but as a temporary "mitigation" (that's why it's named "Mitigate..." and not "Fix..."). The PAX Logging update was pushed out incredibly fast, in fact so fast that there was a solution a few hours before we even started discussing about it on the forum, so I was hoping that karaf would do such a fast update aswell as that would give a whole bunch of additional options on how to proceed (including my favourite: mitigate now, update karaf for the new release on december 20 and revert the mitigation again as then we have a proper fix).

@Flole998
Copy link
Member Author

Given how slowly people upgrade I think w need a 3.1.1 patch. I wouldn't even rule out a 3.0.1 patch for something like this. It only take a quick browsing of Shodan to realize how many people foolishly have a naked openHAB exposed to the Internet.

I checked shodan aswell but look at the versions there..... 2.5, 3.0M1..... You wouldn't reach those with a 3.1.1 or 3.0.1, I think they just leave their instances running and forget about them. I don't think they are afraid of moving from 3.0 to 3.1 and would prefer to move to 3.0.1 instead, so I am not sure if 3.0.1 would actually be installed. It's definitely a great approach to want to reach as many people as possible and patch as many instances at possible, but if someone doesn't want to update or simply forgets that they have a publicly available openHAB then they will not update, no matter how much effort you put into backporting the fix. I'd rather make an annoucement on the forum for 3.0 and 2.X runners that they should manually change their config files, as that can be done on ANY openHAB version, even if they are running some old milestone and don't want to upgrade for whatever reason.

@digitaldan
Copy link
Contributor

I'm not sure i'm seeing the downside to enabling "formatMsgNoLookups" ? What features would we be loosing ? It's a bit hard to google it right now, the internet is screaming about it as a fix, but not talking about what it disables exactly .

@kaikreuzer
Copy link
Member

I'd rather make an annoucement on the forum for 3.0 and 2.X runners that they should manually change their config files

That's definitely least effort, so I could go with this. Whoever wants to update only has to wait until Dec 20 and whoever wants to fix what they have right now, they can manually fix it themselves.

@Flole998
Copy link
Member Author

I'd rather make an annoucement on the forum for 3.0 and 2.X runners that they should manually change their config files

That's definitely least effort, so I could go with this. Whoever wants to update only has to wait until Dec 20 and whoever wants to fix what they have right now, they can manually fix it themselves.

That was not meant as an "instead of backporting to 3.1" but as an "and backporting to 3.1". The idea was to group users in 2 groups:

  1. "Non-Tech-People" that use openHABian and/or use automatic updates (they get it fixed as a service)
  2. "Tech-People" who want control over their instances and manually update (they need to manually update or manually apply the fix, whichever is easier for them)

@digitaldan
Copy link
Contributor

digitaldan commented Dec 10, 2021

to answer my own question about what it disables
https://logging.apache.org/log4j/2.x/manual/lookups.html

@kaikreuzer
Copy link
Member

Well, the non-tech group is usually also rather slow in doing updates, and if they blindly do the update, they will receive 3.2.0 on Dec 20 anyhow - so for just 10 days, the effort to do a 3.1.1 inbetween does not really seem justified.

@Flole998
Copy link
Member Author

I'm not sure i'm seeing the downside to enabling "formatMsgNoLookups" ? What features would we be loosing ? It's a bit hard to google it right now, the internet is screaming about it as a fix, but not talking about what it disables exactly .

If (and I am not sure that exists in openHAB) you have something like
logger.log("Running as $${env:USER}" )
that would no longer log the user as far as I understand. Works with all the "parameters" mentioned in https://logging.apache.org/log4j/2.x/manual/lookups.html, so you could print time/date like that.

@kaikreuzer
Copy link
Member

I'm very certain that we don't use any such log constructs in openHAB...

@digitaldan
Copy link
Contributor

Yeah, this looks fine to me.

@kaikreuzer
Copy link
Member

Alright, shall we merge then?

@Flole998
Copy link
Member Author

Flole998 commented Dec 10, 2021

Well, the non-tech group is usually also rather slow in doing updates, and if they blindly do the update, they will receive 3.2.0 on Dec 20 anyhow - so for just 10 days, the effort to do a 3.1.1 inbetween does not really seem justified.

If that's the way to go then I would leave this open and hope for a Karaf update.... Merging this only makes sense if this is supposed to be backported aswell IMO. Unless you don't are about merging now and reverting in a week or so. On the other hand this would give additional testing before the release.... which might not be needed at all if Karaf is updated.

I think there first needs to be a clear plan on what to do now, if forum annoucement plus fix on december 20 it is then merging this now doesn't really make sense (I'd wait and hope for a karaf update and then merge this in 2 days on the 13th if there isn't one until then). If that plus backporting is the plan then it would make sense to merge (and revert later on if Karaf is updated). Maybe @digitaldan and @rkoshak as AC members should vote on how to proceed?

@kaikreuzer
Copy link
Member

I do not really count on the Karaf update - as @wborn mentioned, the 4.3.4 seems to have some regressions, so even if the fix is added, the upgrade might be a risky one for us in the last minute before our release.

We should imho rather look if we can do the Pax logging update with this magic exclusion mechanism. If we succeed with that, we can still rollback this PR.
But having it now on the snapshot will at least assure us that telling people in the forum to apply this fix to their instances (that they do not want to update) is a safe one - it is after all our recommendation for everybody that won't move to the latest greatest 3.2.

@digitaldan
Copy link
Contributor

Since this is the fix that is available , i vote to merge it now. If karaf does implement a proper fix, then we can always revert this, but karaf upgrades always need testing so that could take time,

@Flole998
Copy link
Member Author

As I understood @wborn he tested it with OH and it seems to work well. His post was:

Karaf 4.3.4 is currently on vote (see mailing list) and the staged artifacts were created a couple of days ago so it still depends on Pax Logging 2.0.10 (without the fix).

Besides that, Karaf 4.3.4 does seem to work well with OH 3.2 after doing some testing with it this afternoon.

@Flole998
Copy link
Member Author

Since this is the fix that is available , i vote to merge it now. If karaf does implement a proper fix, then we can always revert this, but karaf upgrades always need testing so that could take time,

2 AC members against me who doesn't have to say anything ;) Fair enough, feel free to merge :) I am working on writing a forum post to sum up the manual fix described in the forum right now.

@kaikreuzer
Copy link
Member

Besides that, Karaf 4.3.4 does seem to work well

Oh, I read it as "does not seem to work well" as this is what @wborn usually writes after testing Karaf updates. 😆

@kaikreuzer
Copy link
Member

2 AC members against me

Hey, you are the one who created the PR. I didn't yet see anyone complaining that his PR is getting accepted. 😝

@kaikreuzer kaikreuzer added the bug An unexpected problem or unintended behavior label Dec 10, 2021
@kaikreuzer kaikreuzer added this to the 3.2 milestone Dec 10, 2021
@kaikreuzer kaikreuzer merged commit 18dff9a into openhab:main Dec 10, 2021
@rkoshak
Copy link
Contributor

rkoshak commented Dec 10, 2021

I'd vote too to at least merge it now. If a better fix comes along before 3.2 so much the better.

Based on my experience on the forum I'm not sure those who are slow to upgrade operate with quite the same motivations and decision making on when and how to upgrade. Indeed some user won't even know they are upgrading. Others are lazy about it.

But there is a large contingent of users who deliberately do not update. Those users though would update to a 3.1.1 patch release for something like this though. What I can't say is how many of such users we have to make a judgement on how much extra work it's worth to back port it.

@Flole998
Copy link
Member Author

2 AC members against me

Hey, you are the one who created the PR. I didn't yet see anyone complaining that his PR is getting accepted. 😝

I know 😂 Anyways, it's merged now and there is a plan on how to move forward, so too late to complain anyways and I am happy with that plan, I just wanted it clearly written down so everyone knows how this is handled now, before there were suggestions from you and me but no clear "this is how we do it".

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/log4j-vulnerability/129863/44

@wborn
Copy link
Member

wborn commented Dec 11, 2021

I'm also OK with this quick fix!

The vote for Karaf 4.3.4 was cancelled so the new release will also contain the Pax Logging fix.

See: http://mail-archives.apache.org/mod_mbox/karaf-dev/202112.mbox/%3C16600CA0-E3EF-4EF2-B782-FBE92E81BDE4%40nanthrax.net%3E

Yes so far Karaf 4.3.4 didn't have any issues while testing it. But it also doesn't have that many changes compared to 4.3.3.

@kaikreuzer
Copy link
Member

Sounds indeed as if we get Karaf 4.3.4 with the fix in time for our 3.2 release. 👍

wborn added a commit to wborn/openhab-distro that referenced this pull request Dec 11, 2021
Reverts openhab#1343 because Karaf 4.3.4 uses Pax Logging 2.0.11 which is not vulnerable.

Signed-off-by: Wouter Born <[email protected]>
@wborn
Copy link
Member

wborn commented Dec 11, 2021

I've created a few PRs so we can immediately update to Karaf 4.3.4 when it is released, see #1344.

@wborn wborn added the security label Dec 11, 2021
@ghys
Copy link
Member

ghys commented Dec 11, 2021

Sorry I missed that train - I'm all for the fix, and more generally actively tackling security vulnerabilities. 👍
I had no idea you could do such things with log messages, there might be niche cases where this is useful but I don't think it'll be missed by many, so better disable it.

On the patch update, I was more of the opinion that we should do them and have an easy official remediation for people still on the current supported stable release (upgrade to 3.1.1) instead of relying on people making the fix themselves - but I'm not personally making the effort so... :)

In any case, I believe we should issue a proper advisory/CVE whatever the fix is. We should even do it ASAP, for instance as soon as 3.2RC1 is released and not wait until 3.2 GA as it'll be the first non-snapshot that is not affected.

@kaikreuzer
Copy link
Member

On the patch update, I was more of the opinion that we should do them and have an easy official remediation for people still on the current supported stable release

Ok, I'm trying to do a 3.0.3 and 3.1.1 release with that fix - let's see how it works out...

kaikreuzer pushed a commit that referenced this pull request Dec 11, 2021
@kaikreuzer
Copy link
Member

I succeeded. I cherry-picked this PR to 3.0.x and 3.1.x branches and created new releases 3.0.3 and 3.1.1 from it.
I have also published an advisory for it: GHSA-j99j-qp89-pcfq

kaikreuzer pushed a commit that referenced this pull request Dec 12, 2021
@kaikreuzer
Copy link
Member

I succeeded.

Only half-succeeded. 😢
3.1.1 is fine, but 3.0.3 lacks the fix - a new build for 3.0.4 is running right now.

@wborn
Copy link
Member

wborn commented Dec 12, 2021

I see you updated the version to 3.0.4 in the advisory GHSA-j99j-qp89-pcfq, but the description still references 3.0.3:

The openHAB patch releases 3.0.3 and 3.1.1 contain the mitigation described in this post.

@kaikreuzer
Copy link
Member

Thx, fixed.

wborn added a commit to wborn/openhab-distro that referenced this pull request Dec 14, 2021
Reverts openhab#1343 because Karaf 4.3.4 uses Pax Logging 2.0.11 which is not vulnerable.

Signed-off-by: Wouter Born <[email protected]>
wborn added a commit to wborn/openhab-distro that referenced this pull request Dec 15, 2021
Reverts openhab#1343 because Karaf 4.3.4 uses Pax Logging 2.0.11 which is not vulnerable.

Signed-off-by: Wouter Born <[email protected]>
wborn added a commit to wborn/openhab-distro that referenced this pull request Dec 15, 2021
Reverts openhab#1343 because Karaf 4.3.4 uses Pax Logging 2.0.11 which is not vulnerable.

Signed-off-by: Wouter Born <[email protected]>
wborn added a commit to wborn/openhab-distro that referenced this pull request Dec 15, 2021
Reverts openhab#1343 because Karaf 4.3.4 uses Pax Logging 2.0.11 which is not vulnerable.

Signed-off-by: Wouter Born <[email protected]>
wborn added a commit to wborn/openhab-distro that referenced this pull request Dec 15, 2021
Reverts openhab#1343 because Karaf 4.3.4 uses Pax Logging 2.0.12 which is not vulnerable.

Signed-off-by: Wouter Born <[email protected]>
wborn added a commit to wborn/openhab-distro that referenced this pull request Dec 17, 2021
* Reverts openhab#1343 because Pax Logging 2.0.12 is not vulnerable.
* Excludes Pax Logging 2.0.10 to reduce archive size and to prevent scanner false positives.
* Adds missing new line.

Signed-off-by: Wouter Born <[email protected]>
splatch pushed a commit to splatch/openhab-distro that referenced this pull request Dec 17, 2021
* Reverts openhab#1343 because Pax Logging 2.0.12 is not vulnerable.
* Excludes Pax Logging 2.0.10 to reduce archive size and to prevent scanner false positives.
* Adds missing new line.

Signed-off-by: Wouter Born <[email protected]>
kaikreuzer pushed a commit that referenced this pull request Dec 18, 2021
* Upgrade to Karaf 4.3.4

* Syncs distro customizations with Karaf 4.3.4
* Resolves app runbundles for the new dependencies

For release notes, see:

https://issues.apache.org/jira/secure/ReleaseNote.jspa?projectId=12311140&version=12350547

Karaf 4.3.4 uses Pax Logging 2.0.12 which fixes CVE-2021-44228 and CVE-2021-45046.

See also these advisories:

* Log4j: GHSA-jfh8-c2jp-5v3q, GHSA-7rjr-3q55-vv33
* Pax Logging: GHSA-xxfh-x98p-j8fr

Fixes #1334

* Revert CVE-2021-44228 quick fix

Reverts #1343 because Karaf 4.3.4 uses Pax Logging 2.0.12 which is not vulnerable.

Signed-off-by: Wouter Born <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants